- 
                Notifications
    You must be signed in to change notification settings 
- Fork 162
✨ Video Quality Monitoring adaptation #3822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| ✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 48f23b9 | Docs | Was this helpful? Give us feedback! | 
| Bundles Sizes Evolution
 🚀 CPU Performance
 🧠 Memory Performance
 | 
82b4965    to
    f91ecbf      
    Compare
  
    | export type RumViewEvent = CommonProperties & | ||
| ViewContainerSchema & { | ||
| ViewContainerSchema & | ||
| StreamSchema & { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is Stream supposed to be its own event or part of the RUM View? I'm confused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is it to be part of the RUM View
|  | ||
| const subscription = lifeCycle.subscribe(LifeCycleEventType.RUM_EVENT_COLLECTED, (event): void => { | ||
| if (event.type === 'view' || event.type === 'vital' || !isChildEvent(event)) { | ||
| if (event.type === 'view' || event.type === 'vital' || !isChildEvent(event) || ['stream'].includes(event.type)) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: let's keep things uniform, either use event.type === 'stream' or use a list of event types that are never counted containing view, vital and stream
| lifeCycle.notify(LifeCycleEventType.RUM_EVENT_COLLECTED, serverRumEvent) | ||
|  | ||
| if (rawRumEvent.type === 'stream') { | ||
| const streamEvent = { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: this doesn't look like production ready code..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My knowledge is that the idea is to make it work, and have it tested next week with customers and if no one want's it, to be dropped. That's why I think maybe the integration is not production ready.
But I will move this to a helper function, use the types and unit test it to make it prod ready.
4391415    to
    d52e7e5      
    Compare
  
    8bc9e4a    to
    03c87eb      
    Compare
  
    5190b39    to
    4af6b65      
    Compare
  
    
Motivation
Video Quality Monitoring initiative plans to reuse view events to send stream information. This PR adds those foundational changes. Additionally, there will be an additional PR in the playground where the VQM plugin will be published.
Changes
This PR adds the
rawtypes forstreamthat gets transformed in the assembly step to aviewevent.Test instructions
This PR is difficult to manually test. Changes are planned for a follow-up PR in the playground where the new VQM plugin uses
streamevents.Checklist